Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Setup] Centralize registry manipulation and clean up installation logic #13959

Merged
merged 3 commits into from
Oct 25, 2021

Conversation

yuyoyuppe
Copy link
Collaborator

@yuyoyuppe yuyoyuppe commented Oct 22, 2021

Summary of the Pull Request

What is this about:

  • introduce high-level registry API which could be used both in modules and the installer. it's simple to use from new modules which require a lot of shellex handlers, also simple to switch from perMachine to perUser installation
  • wire up the API with PowerPreview, cleanup and clarify hardcoded CLSIDs
  • cleanup PowerPreview logic and remove obsolete unit tests
  • since PowerRename and ImageResizer do not modify the registry when toggling them on and off (they read .json setting file directly from a context menu handler callback instead), we don't need to move the registry from .wxs to a custom action, however, it could be easily done if it's preferred
  • now bootstrapper always detects if it's not elevated and elevates itself to avoid 2 UAC prompts on update (one for uninstall current version, and another for installing new one)

What is include in the PR:

How does someone test / validate:

  • install PT using bootstrapper, without launching PT verify that HKCR\CLSID contains CLSIDs from modulesRegistry.h, e.g. ddee2b8a-6807-48a6-bb20-2338174ff779
  • toggle PowerPreview modules on and off and make sure they work (thumbnails take a while before updating, you try cleaning cache with rm -Force $env:LOCALAPPDATA\microsoft\windows\explorer\thumbcache* instead of rebooting)
  • also try the steps above with UAC-enabled and as a non-admin user

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubbersteamp LGTM, with some nits.
Regarding the keys, they're still in machine, right?

.github/actions/spell-check/expect.txt Outdated Show resolved Hide resolved
.github/actions/spell-check/expect.txt Outdated Show resolved Hide resolved
@yuyoyuppe
Copy link
Collaborator Author

Regarding the keys, they're still in machine, right?

@jaimecbernardo that's right.

@Jay-o-Way it's just a set of registry keys which are grouped together to be easily (un)applied as part of one operation. see modulesRegistry.h for usage examples. I guess the name makes more sense when it's spelled with its namespace - registry::changeSet, but I'm open to suggestions 🙂

@Jay-o-Way

This comment has been minimized.

@yuyoyuppe
Copy link
Collaborator Author

@Jay-o-Way the typo is now fixed, thanks! Turns out automated fixing of automated spellcheck isn't always a good idea.🙂

Copy link
Collaborator

@SeraphimaZykova SeraphimaZykova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on admin and user accounts, in both cases works well. Verified CLSIDs in the registry and checked if PowerPreview modules work.

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested steps from description. Everything works as expected!

@github-actions
Copy link

github-actions bot commented Oct 25, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • changeset
Previously acknowledged words that are now absent Accessible ALIGNLEFT AUTOAPPEND AUTOSIZECOLUMNS available BEGINLABELEDIT btn CASESENSITIVE checkboxes CHECKCANCELED CIEXYZ coc COLUMNCLICK countslabelrenamingfmt countslabelselectedfmt CRename CTriage dchristensen DISPINFO djsoref docsmsft dogancelik DOUBLEBUFFER dupenv DWLP Entireitemname ENUMITEMS estdir EXCLUDEFILES EXCLUDEFOLDERS EXCLUDESUBFOLDERS EXISTINGIMAGERESIZERPATH EXISTINGPOWERRENAMEEXTPATH EXTENIONONLY EXTENSIONONLY Fody ftp ftps FULLNAME GETDISPINFO GETEMPTYMARKUP gmx HACCEL HDF hdi HDITEM hdlg HDN HDS hitinfo htt ianjoneill IAuto IDrop INDEXTOSTATEIMAGEMASK inprivate INSTALLFOLDER installpowertoys IPreview IProgress ITEMSTATEICONCLICK IThumbnail itsme jakeoeding KERNELBASE listbox LPDWORD LPNMHDR LPNMHEADER LPNMLISTVIEW LPOLESTR LPPOINT lvc LVCF LVCFMT LVHITTESTINFO LVHT LVIF LVIS LVN LVS LVSIL MARQUEEPROGRESS MATCHALLOCCURENCES MATCHMODE mfreadwrite mfuuid MINMAXINFO NAMEONLY Nefario nitroin NMLVEMPTYMARKUP null nunit ONITEM OPTIONSGROUP pcelt pitem plvdi pnm pnmdr pnmlv POINTL polymorpism powertoyswiki ppenum ppsrui PREVIEWGROUP PROGDLG prpui prui Radiobuttons RDW refcount REPLACEWITH Reportx rgelt Rgn SEARCHFOR SEARCHREPLACEGROUP SHAREIMAGELISTS sidepanel SINGLESEL SORTDOWN spamming spdth sppd sppre spsrif spsrui STATEIMAGEMASK systray THEMECHANGED TIMERID titlecase ulazy UPDOWNKEYDROPSLIST USEREGEX windevbuildagents winstore XDiff xia XSmall xunit YDiff
Some files were were automatically ignored

These sample patterns would exclude them:

^src/modules/previewpane/UnitTests-MarkdownPreviewHandler/HelperFiles/MarkdownWithHTMLImageTag\.txt$

You should consider adding them to:

.github/actions/spell-check/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:yuyoyuppe/PowerToys.git repository
on the issue_10126 branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
(cat '.github/actions/spell-check/excludes.txt' - <<EOF
$should_exclude_patterns
EOF
) |grep .|
sort -f |
uniq > '.github/actions/spell-check/excludes.txt.temp' &&
mv '.github/actions/spell-check/excludes.txt.temp' '.github/actions/spell-check/excludes.txt'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/PowerToys/issues/comments/951076126" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  

should_exclude_patterns=$(perl -e '$/=undef;
$_=<>;
exit unless s{(?:You should consider excluding directory paths|You should consider adding them to).*}{}s;
s{.*These sample patterns would exclude them:}{}s;
s{.*\`\`\`([^`]*)\`\`\`.*}{$1}m;
print' < "$comment_body" | grep . || true)

update_files
rm $comment_body
git add -u
If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work!

@yuyoyuppe yuyoyuppe merged commit 5910836 into microsoft:master Oct 25, 2021
@yuyoyuppe yuyoyuppe deleted the issue_10126 branch October 25, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants